Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: introduce flag for trace input to e2e framework #1318

Merged
merged 26 commits into from
Sep 27, 2023
Merged

test: introduce flag for trace input to e2e framework #1318

merged 26 commits into from
Sep 27, 2023

Conversation

p-offtermatt
Copy link
Contributor

@p-offtermatt p-offtermatt commented Sep 20, 2023 •
edited
Loading

Description

Closes: Part of #1265

  • added a new flag --test-file filename::testrunner
  • These changes are made so that the steps and the runner are separated. This is more future proof, and for test-file, specifying the test runner separately is necessary. The test runner for the pre-defined test cases is optional, and if not specified it just uses the first compatible one. I think this will be a useful pattern to have in the future, since we are planning to allow running the same tests in different contexts, e.g. with various configurations

I also refactored the TestRun struct to be called TestConfig - this seems much closer to what it actually represents.
The old name was confusing, because the Steps and the TestRun are different things, adn TestRun really sounds like
it contains the steps, which was not the case.

To review this...

  • check that actions*.go, config.go and state.go only contains the refactor of changing TestRun to TestConfig
  • check that all references to TestRun are changed to TestConfig
  • check the meat of the changes: the behaviour of the new flag in main.go

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • Included the correct type prefix in the PR title
  • Targeted the correct branch (see PR Targeting)
  • Provided a link to the relevant issue or specification
  • Reviewed "Files changed" and left comments if necessary
  • Confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • Confirmed the correct type prefix in the PR title
  • Confirmed all author checklist items have been addressed
  • Confirmed that this PR does not change production code

@p-offtermatt
Add json marshal/unmarshal for test traces
d8dcf54
@p-offtermatt
Remove auto-generated trace files
276e347
@p-offtermatt
Add an example trace to see the input format
28eec48
@p-offtermatt
Add comment in rapid test
ae67d35
@p-offtermatt
Remove duplicated test case
5d894e6
@p-offtermatt
Remove TODO in favor of comment
5dfb63b
@p-offtermatt
Reduce code duplication
0948b05
@p-offtermatt
Run go mod tidy and make format
e3e3e84
@p-offtermatt
Correct testdata directory in gitignore
010bec3
@p-offtermatt
Remove testdata directory
f00e209
@p-offtermatt
Remove testdata from gitignore
70350c2
@p-offtermatt
Add tracehandler_testdata
b02d4f7
@p-offtermatt
Remove example trace, since there are examples in the test data
a519315
@p-offtermatt
Revert unintentional change to .gitignore
9eb0a1f
@p-offtermatt
Remove propType field from generator
2f961cf
@p-offtermatt
Add docstrings to action_rapid test and state_rapid_test to better ex…
760c67a 
…plain what the files are doing
@p-offtermatt
Add flag for reading test traces
e62c9a7
@p-offtermatt p-offtermatt changed the base branch from main to ph/json-marshal September 20, 2023 08:21
@github-actions github-actions bot added C:Testing Assigned automatically by the PR labeler C:Build Assigned automatically by the PR labeler C:CI Assigned automatically by the PR labeler labels Sep 20, 2023
Base automatically changed from ph/json-marshal to main September 25, 2023 08:54
@p-offtermatt p-offtermatt marked this pull request as ready for review September 25, 2023 09:24
@p-offtermatt p-offtermatt requested a review from a team as a code owner September 25, 2023 09:24
MSalopek
MSalopek approved these changes Sep 25, 2023
View reviewed changes
Copy link
Contributor

@MSalopek MSalopek left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

I have no issues with the current work. I'll avoid nitpicking.

One idea that comes to mind as a possible refactor is de-coupling configurations from test actions.
Instead of binding actions to config, each action can take a config as argument. However, that does not have any immediate impact and is quite a large refactor.

@p-offtermatt p-offtermatt mentioned this pull request Sep 25, 2023
Open
@p-offtermatt
Copy link
Contributor Author

Great work!

I have no issues with the current work. I'll avoid nitpicking.

One idea that comes to mind as a possible refactor is de-coupling configurations from test actions. Instead of binding actions to config, each action can take a config as argument. However, that does not have any immediate impact and is quite a large refactor.

Thanks. I agree this is a possible refactor. I think that's something we may want to keep in mind, but I don't think it's important enough to prioritize. I added an issue
that we can keep around to have a bit of context #1325

@p-offtermatt p-offtermatt linked an issue Sep 25, 2023 that may be closed by this pull request
Modify the e2e framework to take traces as an input #1240
Closed
shaspitz
shaspitz approved these changes Sep 26, 2023
View reviewed changes
Copy link
Contributor

@shaspitz shaspitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new naming is great, thanks! W.r.t above comment I don't see any immediate impact of making action funcs accept TestConfig as an argument, instead of making the action funcs methods of TestConfig.

From my understanding, golang does not offer any meaningful differences between funcs accepting an obj, and methods defined on an obj, unless interfaces are involved.

@p-offtermatt p-offtermatt merged commit d2c2950 into main Sep 27, 2023
14 checks passed
@p-offtermatt p-offtermatt deleted the ph/trace-flag branch September 27, 2023 07:14
@p-offtermatt
Copy link
Contributor Author

The new naming is great, thanks! W.r.t above comment I don't see any immediate impact of making action funcs accept TestConfig as an argument, instead of making the action funcs methods of TestConfig.

From my understanding, golang does not offer any meaningful differences between funcs accepting an obj, and methods defined on an obj, unless interfaces are involved.

This is also my understanding. I think it looks slightly off that actions are methods of the TestConfig, so I think the refactor would have some minimal value, but yes functionally I think this changes nothing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaspitz shaspitz shaspitz approved these changes

@MSalopek MSalopek MSalopek approved these changes

Assignees
No one assigned
Labels
C:Build Assigned automatically by the PR labeler C:CI Assigned automatically by the PR labeler C:Testing Assigned automatically by the PR labeler
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Modify the e2e framework to take traces as an input
3 participants
@p-offtermatt @MSalopek @shaspitz